Conversation
PROTOCOL_MAJOR returns a simple major number, rather than the number multiplied by 256. This is what one would reasonably expect from GET_PROTOCOL_MAJOR, but we can't just change a macro in a public header; hence the "rename".
PROTOCOL_MAJOR returns a simple major number, rather than the number multiplied by 256. This is what one would reasonably expect from GET_PROTOCOL_MAJOR, but we can't just change a macro in a public header; hence the "rename".
roberth
added
the
contributor-experience
|
|
||
| #define SERVE_PROTOCOL_VERSION (2 << 8 | 7) | ||
| #define PROTOCOL_MAJOR(x) (((x) & 0xff00) >> 8) | ||
| #define PROTOCOL_MINOR(x) ((x) & 0x00ff) |
There was a problem hiding this comment.
Does anything speak against implementing that as a nicely typed function like this:
constexpr size_t protocol_minor(size_t x) { return x & 0xff; }This way this happens at compile time if possible and no one will ever have to handle problems that occur from entering the wrong signed/unsigned type.
|
|
||
| #define PROTOCOL_VERSION (1 << 8 | 34) | ||
| #define PROTOCOL_MAJOR(x) (((x) & 0xff00) >> 8) | ||
| #define PROTOCOL_MINOR(x) ((x) & 0x00ff) |
There was a problem hiding this comment.
it seems like it would make sense to give the different major/minor macros (or functions if the other suggestion is followed) differing names as this also makes it easier to grep for their usage etc.
There was a problem hiding this comment.
Duh, that was indeed a nice footgun.
In addition to #7523 (comment), could we also deprecate the old macros? Deprecating a macro isn't really possible as far as I know, but we can hack something like:
constexpr size_t deprecated(size_t x) { return x }
#define GET_PROTOCOL_MAJOR(x) (deprecated((x) & 0xff00))
#define GET_PROTOCOL_MINOR(x) (deprecated((x) & 0x00ff))
|
I would just remove the old macros. I think the only use is in Hydra which can be updated easily. |
|
The old macro should stick around for compatibility, as Hydra isn't the only consumer. The new macros are also necessary for library-consuming code that needs to work around certain issues (shouldn't have to, but escape hatches are good). TODO: clean up the implementation code to use a struct |
|
@roberth and I discussed this, and we think it would be good for the C++ code to fully parse the version once into a struct, and then pass that around instead. @roberth also mentioned it would still be good to have a macro to --- in effect --- document the binary format itself separate from Nix's parser implementing it (which is the spirit of these |
Have tried this but failed. I accidentally broke something. I would prefer to just merge this, to provide an alternative to the footgun, and then move on to some more relevant problem solving. |
|
@roberth Sounds good. Do you think you could push your attempt somewhere? I would be happy to take a stab at the follow-up if that helps. |
|
It looks like I did a proper rage quit deleting my edits. It's mostly just a bunch of find and replace, so it's probably best to start over anyway. Sorry for dropping the actual struct bit though :/ EDIT: also this was like a week ago and my reflog is not helping either. |
5 participants
Why
writeDerivedPathsWhat
Most significant change: add
PROTOCOL_MAJOR. All other changes follow from this. From the commit message:PROTOCOL_MAJORreturns a simple major number, rather than thenumber multiplied by 256. This is what one would reasonably
expect from
GET_PROTOCOL_MAJOR, but we can't just change amacro in a public header; hence the "rename".